Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed org.apache.druid.discovery.BrokerClient by switching to org.apache.druid.sql.client.BrokerClient. Also upgraded SegmentLoadStatusFetcherTest to reflect changes. #17470

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

satwik-codeium
Copy link

@satwik-codeium satwik-codeium commented Nov 13, 2024

Description

This PR updates the SegmentLoadStatusFetcher to use the new SQL package's BrokerClient implementation, replacing the deprecated discovery-based BrokerClient. This change improves code maintainability and aligns with Druid's ongoing modernization efforts.

Migrated BrokerClient Implementation

  • Replaced deprecated org.apache.druid.discovery.BrokerClient with org.apache.druid.sql.client.BrokerClient
  • Updated SQL task handling to use proper task state management through SqlTaskStatus
  • Fixed locale-dependent string formatting by using StringUtils.format

Updated Test Framework

  • Refactored SegmentLoadStatusFetcherTest to properly mock SQL task lifecycle
  • Added proper task state transitions (RUNNING → SUCCESS) in test cases
  • Maintained existing test coverage while adapting to new API

Release note

Migrated Multi-Stage Query's segment load status checking to use the new SQL package's BrokerClient, removing dependency on the deprecated discovery-based implementation. This change improves code maintainability and provides better SQL task state handling.


Key changed/added classes in this PR
  • org.apache.druid.msq.exec.SegmentLoadStatusFetcher
  • org.apache.druid.msq.exec.SegmentLoadStatusFetcherTest

This PR has:

  • been self-reviewed.
  • added documentation for modified behaviors.
  • a release note entry in the PR description.
  • added comments explaining the "why" and intent of code changes.
  • modified existing tests to cover new code paths.
  • been tested through unit tests.

Design Decisions:

  1. Task State Management:

    • Switched to using TaskState for explicit state tracking
    • Simplified error handling by properly propagating task status
    • Improved readability by separating state management from query execution
  2. Test Structure:

    • Mock responses now simulate realistic task state transitions
    • Maintained existing test scenarios while adapting to new API
    • Added proper verification of task state changes
  3. Error Handling:

    • Improved error propagation through SqlTaskStatus
    • Added explicit handling of failed tasks
    • Enhanced logging for better debugging

@github-actions github-actions bot added Area - Batch Ingestion Area - Dependencies Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 labels Nov 13, 2024
@satwik-codeium satwik-codeium changed the title Codeium's Cascade just did this!! Codeium'sAI just did this!! Nov 13, 2024
@satwik-codeium satwik-codeium changed the title Codeium'sAI just did this!! Removed org.apache.druid.discovery.BrokerClient by switching to org.apache.druid.sql.client.BrokerClient. Also upgraded SegmentLoadStatusFetcherTest to reflect changes. Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area - Batch Ingestion Area - Dependencies Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant